Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enhance typing of CL requests / beacon/execution payload handling / Dedicated Util Withdrawal + Deposit Request Classes #3398

Merged
merged 7 commits into from
May 4, 2024

Conversation

g11tech
Copy link
Contributor

@g11tech g11tech commented May 4, 2024

No description provided.

Copy link

codecov bot commented May 4, 2024

Codecov Report

Attention: Patch coverage is 50.00000% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 81.79%. Comparing base (61acbd3) to head (ff01018).
Report is 20 commits behind head on master.

❗ Current head ff01018 differs from pull request most recent head 0498b70. Consider uploading reports for the commit 0498b70 to get more accurate results

Additional details and impacted files

Impacted file tree graph

Flag Coverage Δ
client 84.60% <ø> (?)
evm 73.63% <ø> (?)
tx 85.93% <ø> (+0.14%) ⬆️
util 81.40% <50.00%> (?)
wallet 87.25% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Copy link
Member

@jochem-brouwer jochem-brouwer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got two small comments, but I don't think I'm deep enough in this code (yet) to give an in-depth review, so I think better wait until @acolytec3 takes a look 😄

const hasDepositRequests = depositRequests !== undefined && depositRequests !== null
const hasWithdrawalRequests = withdrawalRequests !== undefined && withdrawalRequests !== null
const requests =
hasDepositRequests || hasWithdrawalRequests ? ([] as CLRequest<CLRequestType>[]) : undefined
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this just check if there is a requestsRoot?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well it can check that as well but I thought this check to be more thorough. though i have no strong opinions and can do requestroot check

}

if (this.requests !== undefined) {
for (const request of this.requests) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These requests should be ordered. I'm not sure if a switch / case structure is the "nicest" way to handle this 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the sorting is verified as part of block validations so we don't need to worry and keep things simple here

Copy link
Contributor

@acolytec3 acolytec3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@acolytec3 acolytec3 merged commit 02e5e78 into master May 4, 2024
33 of 34 checks passed
@g11tech g11tech deleted the executionpayloadrequests branch May 5, 2024 04:00
@holgerd77
Copy link
Member

This is really a great one! 👍❤️🙂🙏

@holgerd77 holgerd77 changed the title enhance typing of cl requests and do beacon/execution payload handling Enhance typing of CL requests / beacon/execution payload handling / Dedicated Util Withdrawal + Deposit Request Classes Aug 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants